New connection API and jack-in rewrite #2324
Conversation
|
Could you share the steps we need to take to test this out?
Presumably just Does everything currently work? And amazing work :) |
|
The PR is self contained (
I have been testing the core functionality directly related to the changes. There will be probably plenty of broken parts, but those should be tackled case by case. Should be simple enough though. |
|
@vspinu In the interest of this not going the way it did last time I actually plan to just merge your PR and take it from there. The architecture is sound, we just need to polish things. Before there merge I'd like to ask you to just get the build to pass, everything else we can cleanup and fix later. |
Why is this removed?
It might have been nice if you added a line about everything on this list, as it's not really obvious which became redundant and which you consider questionable. |
Because it would be equivalent to creating a sibling with
Sure. |
cider-connection.el
Outdated
| (let ((sesman-disambiguate-by-relevance t)) | ||
| (sesman-ensure-linked-session 'CIDER))) | ||
|
|
||
| (defun cider-connection-type-for-buffer (&optional buffer) |
There was a problem hiding this comment.
Given we are in refactoring mode, I have always thought of renaming this to something more appropriate and keep the "connection type" for something like "nRepl" or "socket".
This one seems more like a repl type?
There was a problem hiding this comment.
Connection and repl are currently synonyms in the sense that they both refer to the repl buffer; historically that wasn't the case.
It might be a good time to reconsider and rename most of (all?) the ..-connection-.. into ..-repl-.. on this ocassion, but I didn't do that because it would introduce a large amount of non-essential changes.
There was a problem hiding this comment.
Yeah, that's fine by me. We can do it down the road. This still leaves open the question how to differentiate between a connection type (nREPL, socket, unrepl) and a REPL type (clj, cljs) whatever. It'd be nice if we came up with some good terminology for this - e.g. repl-flavour and repl-connection-type or whatever.
There was a problem hiding this comment.
This makes sense to me. cider-repl-type is good for cljs and clj because REPL buffer name contains the type; it's a good association. cider-connection-type and cider-server-type would play well for nREPL, socket, unrepl etc. as those are specifics of the transport protocol which users should not care much about.
There was a problem hiding this comment.
Just refactored:
## repl <> connection overlap cleanup
cider-connections -> cider-repls
cider-current-connection -> cider-current-repl
cider-map-connections -> cider-map-repls
cider-connection-type-for-buffer -> cider-repl-type-for-buffer
cider-repl-set-type -> cider-set-repl-type
cider.el
Outdated
| "Start an nREPL server and connect to it both Clojure and ClojureScript REPLs. | ||
| If PROMPT-PROJECT is t, then prompt for the project for which to | ||
| start the server." | ||
| (defun cider-jack-in-cljcljs (&optional prompt-project soft-cljs-start) |
There was a problem hiding this comment.
I'd name this combo or something cljcljs looks like some type to me. :-)
There was a problem hiding this comment.
I am not a big fan of "combo" or "both" because it's simultaneously not specific enough (combo of what?, both of what?), and unnecessarily restrictive. If some day cider would support clr or stand alone clojure, how would you name those? cljcljs is good because it's consistent with standard postfixes -clj and -cljs. It can be -clj&cljs.
The & separator is particularly useful convention because users can now create custom bootstraps like jack-in-clj&node&feegweel with ease.
|
Just a few general notes on the jack-in changes:
Random feedback, mostly based on my observation that too many commands can be too confusing at some point. |
I preserved that. On C-u both project directory and params are asked for. Just like before.
I think that any type of interactive asking (even y-or-n) is less efficient than several commands with a clean separation of concerns. With this PR we would have 3 keybinding M-j, M-c and M-s plus the shifted versions for cljs. There is virtually zero cognitive burden in having an extra M-s key because all 3 bindings are mnemonic and work by similar pattern. For me, pressing Relatedly I was never fond of "Do you want to reuse repl buffer xyz?" This functionality is no longer there. In fact it's not necessary because of the clean restart implementation. Even when an orphan (process-less or server-less) repl occurred, restarting a connection or a session should bring it back to life.
It full generality it would be a two step process 1) "You have a sessions foo, zoo, bar. Do you want to add a new sibling to one of them or create a new session"? 2) "pick session: foo, zoo, bar?". Even when defaulting to the most relevant current session, one would need an y-or-n question.
With this PR connections and sessions are exited and restarted without extra questions. If a user presses
That entry point is there: |
|
I like the one command with no question approach as well, I see though many people do not, but this can always be customizable. I will try this PR, the approach looks sound. Let me submit an additional requirement as well. More and more often projects will have cljs only REPL requirements. For instance in my case - node server app - I only need the cljs REPL buffer. The clj in in the way and I just close it every time. Maybe this PR solves this already? |
Yes. |
|
Awesome I like the rename |
You'd be suprised how certain people think. Anyways, let's just go like this as it is for now.
I often types some keys by mistake, so I think that's a bad idea, but it's not a big deal. I'll just change this back later if needed.
Great! Still this will need to be wrapped in some Anyways, things are looking good and we're just a green build away from merging this as far as I'm concerned. |
A valid point indeed. But C-c C-q is relatively hard to type; even so for C-c C-s C-q. I think it's worth the risk. In any event, if you plan to change this we need it to reconcile with sesman. It would be really akward if session level commands would differ from connection level in this regard.
It works the other way around, the interactive command
I am on it. It's still quite some work. |
Fair enough.
Ah, got it. Guess I should have read the source first. :-)
👍 |
|
Suggestion: we should make a release right before this gets merged and let people know that melpa snapshot will be considered experimental while this gets hammered out. maybe an announcement in slack and also a post on reddit. Let everyone know we would appreciate feedback and bug reports but that the snapshot will be more experimental than usual. Lots of people use the snapshot at work and a heads up about this invasive change would be welcome I think. |
| (cider-refresh--handle-response response log-buffer)) | ||
| conn))) | ||
| :clj 'any-mode))) | ||
| (cider-map-repls :clj |
There was a problem hiding this comment.
This seems strange to me. If you are in a cljs buffer and invoke cider-refresh, it would do nothing to your cljs repl while silently reloading your clj buffers. Shouldn't this be :clj-strict?
There was a problem hiding this comment.
I just swapped the new mapper and didn't question/check the implementation. The old :clj + 'any-mode should be equivalent to the new :clj and old :clj is equivalent to :clj-strict. The latter throws an error if in wrong buffer.
| (if-let* ((repl (or the-repl a-repl))) | ||
| (cider--switch-to-repl-buffer repl set-namespace) | ||
| (user-error "No REPL found")))) | ||
| (let ((repl (or the-repl a-repl))) |
There was a problem hiding this comment.
previously this used buffer-list to get the list of buffers in order of their last usage I believe. I thought that sesman had a way to prioritize this now without scanning all open buffers which can be quite large (sesman-more-relevant-p). Shouldn't this find all connections of the type for the buffer and reduce over them with sesman-more-relevant-p ? and if non are found of the type then reduce over all linked sessions with sesman-more-relevant-p ?
There was a problem hiding this comment.
Buffers don't hold last-usage timestamp so sesman-more-relevant-p has to use buffer-list anyhow. This is unlikely to be an issue in practice because sorting short-circuit on the most recent repl which is commonly located on the top of the list.
There's nothing meaningful to release at this point, otherwise I would have issued some release. Likely the connection revamp will be the only feature of 0.18.
There's no avoiding this with a stable release as well. I assume all non-trivial extensions are going to be broken after this, but as long as the core is stable we should be fine. End users won't really notice that big of a difference, the biggest impact of the change is the change of the API. |
| Immediately mark the REQUEST as done. | ||
| Return the id of the sent message." | ||
| (let* ((conn (cider-current-connection)) | ||
| (defun cider-nrepl-send-unhandled-request (request &optional connection) |
There was a problem hiding this comment.
You've renamed the functions, but you forgot about the params and local variables. It's odd if we refer to something as a repl in one place and a connection at another place.
There was a problem hiding this comment.
Hmm. I think the intent is not to rename all occurrences of connection. Semantics of "connection" is more appealing in many situations. This is the case in this particular example and all other request/response handling functions. Request are sent through a "connection" not a "repl". I renamed only those functions which explicitly deal with the repl object.
It might be a good idea to keep cider-current-connection as an alias for cider-current-repl and use it in places when "connection" is more appropriate.
There was a problem hiding this comment.
Yeah, I'm mostly concerned about code like (let ((conn (cider-current-repl))... and passing repls for conn params. I've grown used to this mess, but for newcomers all of this can be quite confusing.
There was a problem hiding this comment.
When we say "connection" we mean "process" and when we say "REPL" we mean the buffer. The source of the trouble is that all relevant to the connection values are stored as buffer local variables in the repl buffer. Thus, for convenience we pass buffers as connections. If we would use process plist to store connection parameters then we could pass the processes as "connections" and buffers as "repls". In fact we do pass the server process around but still use buffer-local variables to track its state.
I would suggest that we implement this change after this PR. Working with process plists is a superior strategy anyhow. Incidentally, I was half way through into implementing this idea more than two years ago, but then stupidly lost my code in a crash and have never picked it up again.
There was a problem hiding this comment.
Yeah, that sounds like a good strategy.
sesman.el
Outdated
| ;;; Code: | ||
|
|
||
| (require 'project) | ||
| (require 'mule-util) |
There was a problem hiding this comment.
I don't. It's removed in the upstream sesman. I have already moved sesman.el out of cider as I need to prepare it for MELPA and it's quite a long requests back-log over there at the moment.
There was a problem hiding this comment.
Btw, would mind hosting the upstream version under clojure-emacs? As I told you before - a third party package I can't fix myself directly if needed is a big liability, especially if it's at the heart of CIDER.
I've good a good relationship with the MELPA maintainers, so hopefully we'll be able to speed along the process. Anyways, I'm guessing the initial merge is going to use this version of sesman.el committed here.
There was a problem hiding this comment.
Btw, would mind hosting the upstream version under clojure-emacs?
I don't want to tighten it with any specific project. Immediately after CIDER I will integrate it with ESS and hopefully others will follow. I can give you owner rights to the repo if sustainability is a concern.
There was a problem hiding this comment.
Yeah, making a co-owner of the project would be enough. I proposed clojure-emacs mostly because it gives projects bigger visibility and there are many people in the org, but I'm fine either way.
sesman.el
Outdated
| ;; | ||
| ;;; Code: | ||
|
|
||
| (require 'project) |
There was a problem hiding this comment.
I don't think we've implemented any integration with project in clojure-mode/CIDER, so I'm not sure why this is needed.
There was a problem hiding this comment.
It's needed by sesman to create links between sessions and projects. You might not like it, but that's the built-in generic way available from emacs25 to detect projects roots. More and more modes support it and I think clojure-mode should too.
There was a problem hiding this comment.
Yeah, yeah - I'm very well aware what it is, but as everyone knows I have a different idea about how this should be done. :-)
I was mostly surprised you opted to use something we don't really support in clojure-mode.el, meaning it will just fallback to vcs roots.
There was a problem hiding this comment.
Hmm, I just grepped in Emacs's source code and I see none of the built-in modes are using project.el. :-)
There was a problem hiding this comment.
What alternative do you have in mind? IMO, yet another module specific hack along sesman-project-root-function is inferior to what I have done.
For a module which cares only about project root, project.el is an obvious choice. From an outsider prospective, if both clojre-mode and projectile would register their functions with project-find-function it would be a great step towards uniformization.
AFAICS the two alternatives to project.el are projectile, which is too heavy for the purpose at hand, and project-root which hasn't been updated on MELPA since 2011 and doesn't seem to be used much.
There was a problem hiding this comment.
OK, I'll add support for project.el to both projects. My main gripe with project.el is that this does pretty much nothing useful out of the box (apart some basic xref functionality), which really makes me wonder about the purpose of the project. But let's file this under "pointless conversations". :-)
|
Guess we're ready for mass testing and further polish. :-) |
|
Ha. You were fast. I didn't even have time to test it myself after the green light :) Let's dive then! Would be good to open related issues with a common tag (or 0.19 milestone). I will try to address things as we go. |
|
I already see that there are a few obviously broken things. You were warned, don't be surprised if things don't work as expected. |
Will do!
Well, now we got you more testers. :-) I definitely didn't want us to repeat the mistake of the past where we let the PR linger for way too long. Plus this one was already huge and I'd rather us continue with the improvements in a more incremental manner.
I warned people on twitter and slack about the big changes, hopefully only the adventurous people will update before the end of the week. |
|
I am going to put this here, |
|
Thanks. I forgot to declare those. Will fix in the next PR. |
… changes The function `cider-current-repl-buffer` does not exist anymore in cider after PR [syl20bnr#2324](clojure-emacs/cider#2324) was merged. It broke the usage of functions in the clojure layer that sent used `spacemacs//cider-eval-in-repl-no-focus`, which sends forms to be evaluated in the repl buffer without focusing on it. The fix is to use `cider-current-repl` instead of `cider-current-repl-buffer`
… changes The function `cider-current-repl-buffer` does not exist anymore in cider after PR [#2324](clojure-emacs/cider#2324) was merged. It broke the usage of functions in the clojure layer that sent used `spacemacs//cider-eval-in-repl-no-focus`, which sends forms to be evaluated in the repl buffer without focusing on it. The fix is to use `cider-current-repl` instead of `cider-current-repl-buffer`
… changes The function `cider-current-repl-buffer` does not exist anymore in cider after PR [syl20bnr#2324](clojure-emacs/cider#2324) was merged. It broke the usage of functions in the clojure layer that sent used `spacemacs//cider-eval-in-repl-no-focus`, which sends forms to be evaluated in the repl buffer without focusing on it. The fix is to use `cider-current-repl` instead of `cider-current-repl-buffer`
Second attempt on #2069. A brief description of the new functionality follows.
Jack-in/connect
User level commands:
Create new sessions:
C-c M-j: cider-jack-in-clj
C-c M-J: cider-jack-in-cljs
C-c M-c: cider-connect-clj
C-c M-C: cider-connect-cljs
Add new REPLs to the current session:
C-c M-s: cider-connect-sibling-clj
C-c M-S: cider-connect-sibling-cljs
cider-jack-in-clojurescriptno longer creates two repls, only the cljs replclj repl has no longer a special status of the "main" repl. All repls within a session share same server and are siblings of each other. You can create as many clj and cljs siblings as you want from any repl.
Creation of the client is no longer tightly bounded with the nrepl-server startup. The dynamic communication mechanism between jack-in and nrepl-server filter has been replace by a simple
on-port-callback.New Connection and Session Management API
Connections (aka REPLs) are grouped in sesman sessions. Sibling repls are added to the current session.
Cider connection commands (
cider-quit,cider-restart,cider-display-connection-info) have been refactored to operate exclusively on the connection level.Sesman commands operate on the whole session:
Associations (links) between current context (buffer, directory, project) are governed by sesman and could be formed only on the session level.
In a nutshell, session can be linked to projects, directories and buffers. Buffer link have precedence over directory links, and directory have precedence over project links.
When a sesman session is registered it's automatically linked with the lowest priority context (project, or directory if no project found). By default (configured with
sesman-1-to-1-links) multiple sessions can be linked with a project or a directory, but only one session can be linked with a buffer.Cider functionality (eval, completion, repl-switching etc) operate on linked sessions. When there are multiple linked sessions ambiguity is automatically resolved by the recency of the REPL buffers (configured with
sesman-disambiguate-by-relevance).Show info on current links with
C-c C-s l. Show info on current, linked or all sessions withC-c C-s i.Micro-management of the server is not allowed (it's not useful and would complicate UI). All repls within a session share a server. Server can be either remote (
cider-connect) or local (bootstraped within the emacs duringcider-jack-in-xyz).In case of the local server, when the last connection is killed the server is automatically killed.
cider-restartrestarts the connection but not the server.sesman-restartrestarts the server and all the connections.This PR should still be considered WIP. Would be great if people could pull and test in the upcoming weeks. Conceptual, API and UI issues is the main focus right now. Minor issues with docs, tests etc will be fixed in the later stage once the dust has settled.
To try out (assuming you are loading cider from source):
At least two issues I still plan to tackle here:
A tot of no longer necessary or questionable functionality has been removed. The goal is to start from scratch and add only what is really necessary. I am listing all the removed functions for the ease of lookup through the github interface.
Removed:
Connection Browser Functionality:
Renamed: